Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make the transit networks time aware #83

Closed
wants to merge 6 commits into from

Conversation

bouzaghrane
Copy link

What

This PR is to add capability to create time-aware urbanaccess transit and walk networks.

This is to be able to include the active stops within a time interval after the end of the specified time range.
@smmaurer smmaurer self-requested a review February 15, 2021 22:40
@sablanchard sablanchard self-requested a review February 16, 2021 17:02
Added argument in create transit and route_type_to_edge functions.
+ adding documentation
@bouzaghrane
Copy link
Author

It's unclear to me why checks on my commits keep on failing. Is it a PEP8 issue? It seems like all other tests pass. @sablanchard @smmaurer

@smmaurer smmaurer marked this pull request as draft March 1, 2021 21:36
@smmaurer
Copy link
Member

smmaurer commented Mar 1, 2021

@bouzaghrane Yup, looks like just PEP8/pycodestyle. Here are the lines that get flagged:
https://travis-ci.org/github/UDST/urbanaccess/jobs/761002604#L1258-L1284

You can do pip install pycodestyle and run it locally too, which makes it easier to confirm that everything clears. From the root directory of the repo just run pycodestyle urbanaccess

@bouzaghrane
Copy link
Author

Looks like the checks passed for all but python 3.5. Wondering why..

@smmaurer
Copy link
Member

smmaurer commented Mar 2, 2021

It looks like an installation problem that's unrelated to these changes -- Conda doesn't find Python 3.5-compatible versions of the dependencies, so the tests can't continue.

I'll work on diagnosing this, and we can either patch the Travis script or switch over to GitHub Actions like we've done for Pandana, ChoiceModels, and UrbanSim Templates..

@smmaurer
Copy link
Member

smmaurer commented Mar 8, 2021

Hi @sablanchard and @bouzaghrane!

Sam, Amine mentioned that this is ready for review. Thanks Amine!!

The changes in this PR are to the create_transit_net() function: (1) adding a time_aware parameter that if True adds arrival and departure time columns to the output network, and (2) adding a timerange_pad parameter to optionally extend the timerange by a particular amount.

There are a couple of reasons for the first addition. Including schedule times allows us to create a single large integrated network that we can later break into time-of-day segments on the fly as needed. It also paves the way to potentially include trip schedules in the routing itself.

The reasoning behind timerange_pad is related. If we have an 8am-10am network, we're not guaranteed to be able to route an agent who leaves at 9:30. It will only work if their endpoint can be reached before 10am. Padding the time range would guarantee that we could find routes for every agent who departed within the notional time window. For example, the 8am-10am network could actually be 8am-12pm, and every agent leaving between 8 and 10 would complete their trips. It's helpful to think of the padding separately from the original time range because the optimal padding will vary from region to region.

Feedback from me:

  • The new function arguments should probably go at the end of the existing argument list. This ensures we won't break existing code that calls the function with positional (rather than named) arguments.
  • Let's add a unit test confirming that the time_aware parameter works as designed. Looks like it could go here: test_gtfs_network.py
  • And i'll open a separate PR to fix the test glitch discussed above

@sablanchard, any additional feedback from your side?

@sablanchard
Copy link
Contributor

Working with @bouzaghrane offline we decided to implement the functionality in this PR in a different way in this new PR: #87 thus this PR is superseded by #87 and will be closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants